feat: add `CheckpointLogReplayProcessor` in new `checkpoints` mod #744

sebastiantia · 2025-03-13T20:47:27Z

What changes are proposed in this pull request?

resolves #743

When the checkpoint API is called, we need to return all actions to be included in the checkpoint file for the engine to write. The returned actions will be EngineData batches, where each batch has an accompanying selection vector which informs the engine of which actions in the batch to write/not write to the checkpoint file.

To generate this filtered actions iterator:

This PR introduces the CheckpointLogReplayProcessor which implements the LogReplayProcessor trait. This processor is applied to an iterator of action batches read from the log segment with the new checkpoint_actions_iter method, in order to transform the input actions iterator into an iterator of FilteredEngineData, which includes the log data accompanied with a selection vector indicating which rows should be included in the checkpoint file.

How was this change tested?

These tests test the application of the CheckpointVisitor over multiple batches. The visiting of individual batches is already tested.
test_checkpoint_actions_iter_non_file_actions
test_checkpoint_actions_iter_file_actions
test_checkpoint_actions_iter_file_actions_with_deletion_vectors

codecov · 2025-03-13T20:51:23Z

Codecov Report

Attention: Patch coverage is 89.38547% with 19 lines in your changes missing coverage. Please review.

Project coverage is 85.06%. Comparing base (929ac08) to head (411b2c4).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/checkpoint/log_replay.rs	90.64%	3 Missing and 13 partials ⚠️
kernel/src/scan/log_replay.rs	25.00%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #744      +/-   ##
==========================================
+ Coverage   85.03%   85.06%   +0.02%     
==========================================
  Files          84       84              
  Lines       20656    20802     +146     
  Branches    20656    20802     +146     
==========================================
+ Hits        17565    17695     +130     
- Misses       2226     2229       +3     
- Partials      865      878      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zachschuermann

flushing a few comments. can we (1) describe the overall requirements/flow and (2) if we decide to refactor, have a quick design on the refactor and what it enables and then open a separate PR

kernel/src/actions/visitors.rs

kernel/src/checkpoints/log_replay.rs

kernel/src/log_replay.rs

zachschuermann

dropped a few comments, general direction looks good. let's split out the new LogReplayProcessor trait into a refactor PR first (and include some description for the motivation - could also just do a new PR that links here to show how it's used)

kernel/src/checkpoints/log_replay.rs

sebastiantia · 2025-04-10T23:12:07Z

kernel/src/checkpoint/log_replay.rs

+            // Remove action for file1 with a different deletion vector
+            r#"{"remove":{"path":"file1","deletionTimestamp":100,"dataChange":true,"deletionVector":{"storageType":"TWO","pathOrInlineDv":"dv2","offset":1,"sizeInBytes":36,"cardinality":2}}}"#,
+            // Remove action for file1 with another different deletion vector
+            r#"{"remove":{"path":"file1","deletionTimestamp":100,"dataChange":true,"deletionVector":{"storageType":"THREE","pathOrInlineDv":"dv3","offset":1,"sizeInBytes":36,"cardinality":2}}}"#,


Updated this single-batch from a single-commit file test case as it's not a valid scenario:

"The intersection of the primary keys in the add collection and remove collection must be empty."

Instead we test repeating primary keys spanning across multiple commit files below in test_checkpoint_actions_iter_file_actions_with_deletion_vectors

sebastiantia · 2025-04-10T23:21:52Z

kernel/src/scan/log_replay.rs


        // TODO: Teach expression eval to respect the selection vector we just computed so carefully!
-        let result = self.add_transform.evaluate(actions_batch)?;
+        let result = self.add_transform.evaluate(actions_batch.as_ref())?;


This evaluation returns a Box<EngineData> (mentioned in reasoning for updating parameter type for process_actions_batch in other comment)

zachschuermann

still need to review tests but looks good!

kernel/src/checkpoint/log_replay.rs

nicklan

lgtm, with a nit about not panicing

sebastiantia · 2025-04-11T23:22:35Z

kernel/src/log_replay.rs

-    /// * `i` - Index position in the data structure to examine
-    /// * `getters` - Collection of data getter implementations used to access the data
-    /// * `skip_removes` - Whether to skip remove actions when extracting file actions
+    /// # Parameters


minor formatting changes

zachschuermann

LGTM with a few nits + ask on testing - if we need more tests I think fine to unblock this and open a follow-up

kernel/src/checkpoint/log_replay.rs

kernel/src/log_replay.rs

zachschuermann · 2025-04-12T00:05:50Z

kernel/src/checkpoint/log_replay.rs

+            // Duplicates that should be skipped
+            r#"{"protocol":{"minReaderVersion":2,"minWriterVersion":3}}"#,
+            r#"{"metaData":{"id":"test2","format":{"provider":"parquet","options":{}},"schemaString":"{\"type\":\"struct\",\"fields\":[{\"name\":\"c1\",\"type\":\"integer\",\"nullable\":true,\"metadata\":{}},{\"name\":\"c2\",\"type\":\"string\",\"nullable\":true,\"metadata\":{}},{\"name\":\"c3\",\"type\":\"integer\",\"nullable\":true,\"metadata\":{}}]}","partitionColumns":["c1","c2"],"configuration":{},"createdTime":1670892997849}}"#,
+            r#"{"txn":{"appId":"app1","version":1,"lastUpdated":123456789}}"#,


should we check the latest is returned by making this version different than above?

Right below this line is another txn action with a unique appId which covers the scenario you mentioned

zachschuermann · 2025-04-12T00:07:59Z

kernel/src/checkpoint/log_replay.rs

+        assert_eq!(add_actions, 2);
+
+        Ok(())
+    }


are tests exhaustive? let's not block this PR but perhaps a follow up if we need to cover:

remove tombstones before/after expiration

we skip CommitInfo, CDC, CheckpointMetadata, Sidecar

I think both of your mentioned scenarios are sufficiently tested in the existing visitor level unit tests:

test_checkpoint_visitor_boundary_cases_for_tombstone_expiration

test_checkpoint_visitor – this test already verifies the exclusion of CommitInfo, cdc, and sidecar actions. (I'll update this test in this PR to also include the checkpointMetadata action in the input batch.)

I'm not sure it's worthwhile to add additional tests specifically for cross-batch behavior, but happy to revisit if you think there's value in doing so, let me know!

zachschuermann · 2025-04-12T00:09:45Z

@sebastiantia this isn't a breaking change right? let's remove label if it was accidentally/incorrectly added before

sebastiantia added 4 commits March 12, 2025 10:16

introduce visitors

435302e

remove pub

e500a10

assert! instead of assert_eq with bool

19733cd

log replay for checkpoints

87c9f31

sebastiantia requested review from nicklan, scovich and zachschuermann March 13, 2025 20:47

github-actions bot assigned sebastiantia Mar 13, 2025

sebastiantia added 4 commits March 13, 2025 15:29

rename & some clean up

db5ccd0

remove new path for now

42c08c1

merge non file action visitor tests

f91baeb

mvp for refactor

9fdfba7

sebastiantia changed the title ~~feat: add V1CheckpointLogReplayScanner in new checkpoints mod~~ feat: add CheckpointLogReplayScanner in new checkpoints mod Mar 24, 2025

these github action checks clog my screen

d420fd1

zachschuermann reviewed Mar 25, 2025

View reviewed changes

kernel/src/actions/visitors.rs Outdated Show resolved Hide resolved

kernel/src/checkpoints/log_replay.rs Outdated Show resolved Hide resolved

kernel/src/log_replay.rs Outdated Show resolved Hide resolved

kernel/src/log_replay.rs Show resolved Hide resolved

sebastiantia added 10 commits March 25, 2025 14:34

base file actions struct

9e0e048

combine visitors

303444b

fmt

5dbc924

remove old code

b793961

move FileActionKey

508976f

Merge branch 'main' into checkpoint-visitors

bccaa17

merge

a23d7cb

fix whitespace

0160ef1

remove old code

aae7046

refactor more

f574370

zachschuermann reviewed Mar 26, 2025

View reviewed changes

kernel/src/checkpoints/log_replay.rs Outdated Show resolved Hide resolved

kernel/src/checkpoints/log_replay.rs Outdated Show resolved Hide resolved

kernel/src/checkpoints/log_replay.rs Outdated Show resolved Hide resolved

kernel/src/checkpoints/log_replay.rs Outdated Show resolved Hide resolved

sebastiantia added 2 commits March 26, 2025 14:10

refactor

a618833

more docs

7da74b2

sebastiantia mentioned this pull request Mar 26, 2025

refactor: extract deduplication logic from AddRemoveDedupVisitor into embeddable FileActionsDeduplicator #769

Merged

sebastiantia added 3 commits April 10, 2025 14:10

merge

0b609d5

docs

ab0a373

test coverage

9a9697a

sebastiantia removed the breaking-change Change that require a major version bump label Apr 10, 2025

sebastiantia commented Apr 10, 2025

View reviewed changes

sebastiantia requested a review from OussamaSaoudi April 10, 2025 23:14

doc

c7630a3

sebastiantia commented Apr 10, 2025

View reviewed changes

sebastiantia mentioned this pull request Apr 11, 2025

feat: add Snapshot::checkpoint() & Table::checkpoint() API #797

Merged

zachschuermann reviewed Apr 11, 2025

View reviewed changes

nicklan reviewed Apr 11, 2025

View reviewed changes

kernel/src/checkpoint/log_replay.rs Outdated Show resolved Hide resolved

nicklan approved these changes Apr 11, 2025

View reviewed changes

review

48a0153

sebastiantia requested a review from zachschuermann April 11, 2025 23:20

sebastiantia commented Apr 11, 2025

View reviewed changes

schema spec

4a1a1dd

github-actions bot added the breaking-change Change that require a major version bump label Apr 11, 2025

sebastiantia added 2 commits April 11, 2025 16:26

pub crate

4d48a8a

forgot to include this file

411b2c4

zachschuermann approved these changes Apr 12, 2025

View reviewed changes

sebastiantia removed the breaking-change Change that require a major version bump label Apr 14, 2025

sebastiantia and others added 2 commits April 14, 2025 12:16

review

48d529d

Merge branch 'main' into checkpoint-replay

6a672d8

This was referenced Apr 14, 2025

[tracking issue] checkpoint write support #499

Closed

[tracking issue] Single-file classic-named V1 & V2 checkpoint write support #736

Closed

sebastiantia merged commit 9f5671c into delta-io:main Apr 14, 2025
18 of 19 checks passed

sebastiantia deleted the checkpoint-replay branch April 14, 2025 20:04

sebastiantia restored the checkpoint-replay branch April 14, 2025 20:24

sebastiantia deleted the checkpoint-replay branch April 14, 2025 20:24

feat: add CheckpointLogReplayProcessor in new checkpoints mod #744

feat: add CheckpointLogReplayProcessor in new checkpoints mod #744

Uh oh!

Conversation

sebastiantia commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are proposed in this pull request?

How was this change tested?

Uh oh!

codecov bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sebastiantia Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastiantia Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

sebastiantia Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zachschuermann Apr 12, 2025

Choose a reason for hiding this comment

Uh oh!

sebastiantia Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

zachschuermann Apr 12, 2025

Choose a reason for hiding this comment

Uh oh!

sebastiantia Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

zachschuermann commented Apr 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: add `CheckpointLogReplayProcessor` in new `checkpoints` mod #744

feat: add `CheckpointLogReplayProcessor` in new `checkpoints` mod #744

sebastiantia commented Mar 13, 2025 •

edited

Loading

codecov bot commented Mar 13, 2025 •

edited

Loading

sebastiantia Apr 10, 2025 •

edited

Loading